Update Actions w/ new bin naming#29
Merged
ServeurpersoCom merged 6 commits intoServeurpersoCom:masterfrom Mar 16, 2026
Merged
Conversation
Updated packaging process for macOS binaries to include rpath and additional libraries. Signed-off-by: Lorenzo Mangani <lorenzo.mangani@gmail.com>
Signed-off-by: Lorenzo Mangani <lorenzo.mangani@gmail.com>
Signed-off-by: Lorenzo Mangani <lorenzo.mangani@gmail.com>
Signed-off-by: Lorenzo Mangani <lorenzo.mangani@gmail.com>
Contributor
📝 WalkthroughWalkthroughSwaps smoke-test binaries (replace Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
70-76:⚠️ Potential issue | 🟠 Major
nprocis not available on macOS.Line 76 uses
$(nproc)which is a Linux-specific utility. On macOS, this command will fail. The CI workflow correctly uses$(sysctl -n hw.ncpu)for macOS (see line 36 of ci-build.yml).🐛 Proposed fix
- name: Configure & Build (Linux / macOS) if: runner.os == 'macOS' run: | mkdir build cd build cmake .. - cmake --build . --config Release -j "$(nproc)" + cmake --build . --config Release -j "$(sysctl -n hw.ncpu)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 70 - 76, The macOS build step in the "Configure & Build (Linux / macOS)" job uses the Linux-only "$(nproc)"; change the macOS command in that run block (the Configure & Build step) to use the macOS CPU count command "$(sysctl -n hw.ncpu)" (or detect platform and choose between nproc and sysctl) so the cmake --build invocation uses the correct parallelism on macOS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 127-134: Fix the typo in the copy command: replace the incorrect
glob "ace-ace-*" with "ace-*" so the cp -P command copies the actual ace
binaries (ace-*), quantize, neural-codec, mp3-codec, libacestep*.a and
libggml*.dylib into ../dist/; update the cp invocation that currently references
"ace-ace-*" to use "ace-*" (the rest of the command remains unchanged).
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 70-76: The macOS build step in the "Configure & Build (Linux /
macOS)" job uses the Linux-only "$(nproc)"; change the macOS command in that run
block (the Configure & Build step) to use the macOS CPU count command "$(sysctl
-n hw.ncpu)" (or detect platform and choose between nproc and sysctl) so the
cmake --build invocation uses the correct parallelism on macOS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 881487a0-124b-419d-8211-a4cac27bc938
📒 Files selected for processing (2)
.github/workflows/ci-build.yml.github/workflows/release.yml
Signed-off-by: Lorenzo Mangani <lorenzo.mangani@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
127-132: Prefer explicit macOS binary list overace-*wildcard for release determinism.
ace-*in bothinstall_name_toolandcpis broader than needed and can accidentally include non-release matches, making packaging brittle.♻️ Proposed hardening
cd build - for bin in ace-* quantize neural-codec mp3-codec; do + bins=(ace-lm ace-synth ace-understand quantize neural-codec mp3-codec) + for bin in "${bins[@]}"; do install_name_tool -add_rpath `@executable_path` "$bin" done - cp -P ace-* quantize neural-codec mp3-codec libacestep*.a libggml*.dylib ../dist/ + cp -P "${bins[@]}" libacestep*.a libggml*.dylib ../dist/ cd ..🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 127 - 132, The release step uses a broad ace-* wildcard in the install_name_tool loop and cp command which can pull unintended files; replace the wildcard with an explicit, deterministic list of macOS binaries (the exact names used in the build) in both the for-loop that calls install_name_tool and the cp -P command so only intended artifacts are processed and packaged (update the install_name_tool loop and the cp invocation to reference that explicit list instead of ace-*).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 140-143: The Windows packaging block (the $bins array and foreach
($b in $bins) Copy-Item "build\Release\$b.exe" dist\) is unreachable because the
workflow matrix only includes Linux and macOS runners; either add a Windows
entry to the job matrix so these steps run on a Windows runner, or remove/guard
this Windows-specific packaging code behind a condition (e.g., if: runner.os ==
'Windows') so it only executes when a Windows runner (matrix entry) is present.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 127-132: The release step uses a broad ace-* wildcard in the
install_name_tool loop and cp command which can pull unintended files; replace
the wildcard with an explicit, deterministic list of macOS binaries (the exact
names used in the build) in both the for-loop that calls install_name_tool and
the cp -P command so only intended artifacts are processed and packaged (update
the install_name_tool loop and the cp invocation to reference that explicit list
instead of ace-*).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2f835f8-f109-447d-ac34-64c1ecfa6bb2
📒 Files selected for processing (1)
.github/workflows/release.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit